feat(module-state): add configurable pull scheduling with watchdog fallback - #5226
Conversation
…llback Add a `pull` option to PouchDbSyncStorage so remote-change polling can run on a schedule instead of a single continuous db.sync() connection, and switch createDefaultStorage() over to it. - mode: 'live' (default, unchanged) - continuous bidirectional db.sync(). - mode: 'interval' - push stays live via db.replicate.to; pull runs as one-shot db.replicate.from calls on a timer (+ on focus). - mode: 'visible-interval' - same as 'interval', but skips the timer tick while the tab is hidden (Page Visibility API). Each one-shot pull is guarded by a watchdog timeout (syncOptions.timeout + 5s) that force-cancels and releases the in-flight guard if PouchDB never fires 'complete'/'error' nor settles its thenable interface - otherwise a single hung poll would wedge every later scheduled pull into a silent no-op skip. createDefaultStorage is now also exported from @equinor/fusion-framework-module-state/default-storage so callers can reuse the framework's default remote-resolution behavior with custom pull overrides. The app-react-state cookbook uses this to preview with a 10s interval instead of the 60s production default.
🦋 Changeset detectedLatest commit: eff8ef4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
🟡 Changes recommended
Active pulls outlive disposal, fallback rejections can be hidden, and the watchdog test currently fails against its incomplete mock.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds configurable pull scheduling to module-state while keeping push replication live.
Changes:
- Adds interval and visibility-aware pull modes with watchdog recovery.
- Exposes configurable default storage and polling events.
- Updates cookbook preview behavior and tests.
File summaries
| File | Description |
|---|---|
packages/modules/state/src/storage/PouchDbSyncStorage.ts |
Implements scheduled pulls, live push, and watchdog handling. |
packages/modules/state/src/storage/observe-pouch-db-replicate.ts |
Maps directional replication events to state events. |
packages/modules/state/src/storage/index.ts |
Exports pull options. |
packages/modules/state/src/events/StateSyncPollEvent.ts |
Defines polling events. |
packages/modules/state/src/events/index.ts |
Registers polling in sync event unions. |
packages/modules/state/src/create-default-storage.ts |
Uses visibility-aware polling by default. |
packages/modules/state/src/__tests__/PouchDbSyncStorage.test.ts |
Tests interval replication and watchdog behavior. |
packages/modules/state/package.json |
Adds the default-storage export path. |
cookbooks/app-react-state/src/config.ts |
Configures a shorter preview interval. |
cookbooks/app-react-state/src/components/SyncEvents/SyncStatusIndicator.tsx |
Displays polling status. |
.changeset/module-state_interval-pull.md |
Documents the module-state feature. |
.changeset/cookbook-app-react-state_poll-preview.md |
Documents cookbook changes. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…hdog test mock A watchdog-cancelled (or any cancelled) replication fires PouchDB's 'complete' event with no `docs` at all, crashing observePouchDbReplicate's result parsing with `Cannot read properties of undefined (reading 'map')`. Default to an empty array instead. Also add the missing `removeListener` mock to the watchdog test's hung replication stub - RxJS's teardown calls it for every event registered via `on()`, and its absence was throwing an UnsubscriptionError that left fake timers active and timed out the next test's afterEach hook.
…t pull for teardown - pull.then() previously discarded the rejection reason when it was the only signal a pull failed - finish() now accepts an optional error and emits an onStateSync.error event for it. - The active one-shot pull's cancel and its replication-event subscription are now registered via _addTeardown() so disposing the storage mid-pull cancels them instead of leaving them running past the storage's lifetime. - Also applies biome's formatting suggestions across this file (line wrapping for multi-arg calls and long conditions).
Adds a test proving 'visible-interval' pull mode skips interval ticks while the tab is hidden, then triggers exactly one catch-up pull on returning to visible (not one per missed tick). Also picks up biome's quote-style fix on an existing test title in this file.
…val test's complete() trigger
The fake replication's complete() helper was invoking the 'complete' handler
with no argument, but observePouchDbReplicate's onComplete reads change.docs
directly - this crashed with 'Cannot read properties of undefined (reading
docs)' in CI. Now passes a minimal { docs: [] } result, matching what a real
PouchDB 'complete' event provides.
There was a problem hiding this comment.
🟡 Changes recommended
Public sync() can bypass interval mode, and the watchdog can cancel healthy long-running pulls.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
packages/modules/state/src/storage/PouchDbSyncStorage.ts:94
- This interval-mode branch does not prevent the existing public
sync()method from later starting a bidirectionaldb.sync(). Calling it creates a continuous pull plus a second live push, bypasses#pullInFlight, and violates the stated single-pull invariant; makesync()honor the configured mode or stop the scheduled pull/live push before switching modes.
if ((this.#pull.mode ?? PullMode.Live) === PullMode.Live) {
this.sync();
} else {
this._startLivePush();
this._schedulePulling();
packages/modules/state/src/storage/PouchDbSyncStorage.ts:323
- This is a total-duration deadline rather than a hung-replication watchdog. A healthy pull with multiple requests or batches can run longer than
timeout + 5s—the preceding comment notes thattimeoutonly bounds each underlying request—so it will be canceled mid-progress every cycle; use an inactivity watchdog that is refreshed by replication progress, or a separately configurable total deadline.
const watchdogMs =
(typeof this.#syncOptions.timeout === 'number' ? this.#syncOptions.timeout : 30000) + 5000;
const watchdog = setTimeout(() => {
pull.cancel();
finish();
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…e an inactivity watchdog - Calling the public sync() while pull.mode is 'interval'/'visible-interval' previously started a second, competing continuous pull and live push instead of taking over from them. sync()/_sync() now stops the live push and scheduled pulling first. - The pull watchdog was a fixed total-duration deadline (timeout + 5s), so a healthy pull spanning multiple batches would get force-cancelled every cycle. It now rearms on each replication 'change' event, only firing once a pull goes fully silent.
…ver of non-live mode - pull watchdog: proves repeated 'change' progress keeps a healthy pull alive well past the old total-duration deadline, and that it still fires once the pull goes silent. - public sync(): proves calling it while pull.mode is 'interval' cancels the live push and stops scheduled pulling instead of running alongside them.
There was a problem hiding this comment.
🟡 Changes recommended
Active pulls are not fully stopped during replacement or disposal, and the widened public event union needs breaking-change handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
packages/modules/state/src/storage/PouchDbSyncStorage.ts:102
- This stopper cancels the live push and future triggers, but it does not cancel a one-shot pull already running. Calling public
sync()during the initial/timer pull therefore starts bidirectional sync alongside that pull until it completes or reaches the watchdog, contradicting the replacement invariant; track the active pull cleanup and invoke it here as well.
this.#stopNonLivePull = () => {
push.cancel();
stopScheduledPulling();
};
packages/modules/state/src/storage/PouchDbSyncStorage.ts:292
- These teardown registrations do not actually clear the watchdog or remove the direct
complete/error/changelisteners added below. If disposal cancels a hung replication that emits no terminal event—the failure mode the watchdog handles—the timer and listeners remain alive until the watchdog fires after disposal; register a teardown that calls the sharedfinish()/cleanup path directly.
const removePullTeardown = this._addTeardown(() => pull.cancel());
const removeSubscriptionTeardown = this._addTeardown(subscription);
packages/modules/state/package.json:24
- This new consumer entry point and its pull-scheduling API are absent from
packages/modules/state/README.md(the README contains noPouchDbSyncStorage,createDefaultStorage, or pull-mode documentation), despite the PR checklist marking user-facing docs updated. Add persistent usage/default documentation there; a changeset alone will not remain in the package API guide.
"./default-storage": {
"import": "./dist/esm/create-default-storage.js",
"types": "./dist/types/create-default-storage.d.ts"
},
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…rsedes non-live mode #stopNonLivePull previously only cancelled the live push and future scheduled pulls, not a pull already in flight - calling sync() mid-poll would run bidirectional sync alongside it until the pull completed or the watchdog forced it closed. It now also cancels the active pull via a tracked #cancelActivePull, and disposal routes through the same finish() cleanup (clearing the watchdog and direct listeners) instead of just calling pull.cancel().
…eaking release Adding the new onStateSync.poll event widened the exported StateSyncEventType/StateSyncEvent union, which is source-breaking for consumers with an exhaustive switch/never check over sync events. Bump from minor to major and document the required migration.
…ateDefaultStorage The new /default-storage export and pull-scheduling API (pull.mode 'live'/'interval'/ 'visible-interval') had no persistent documentation - only a changeset. Add a Storage guide section covering both, alongside the onStateSync.poll event it dispatches.
There was a problem hiding this comment.
🟡 Changes recommended
Teardown mutation can leave polling active after disposal, and scheduled replication drops valid direction-specific sync options.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (5)
packages/modules/state/src/storage/PouchDbSyncStorage.ts:301
finish()removes both callbacks from the same teardown array thatPouchDbStorage[Symbol.dispose]()is iterating. When disposal invokes the active-pull teardown, these splices remove the current and next entries, so the iterator skips the following timer teardown registered by_schedulePulling()and interval polling continues after disposal. Make disposal iterate a snapshot/clear the array, or avoid mutating the teardown collection from inside a teardown callback.
removePullTeardown();
removeSubscriptionTeardown();
packages/modules/state/src/storage/PouchDbSyncStorage.ts:359
- After
finish()setssettledand clears the watchdog, a late or currently-dispatchingchangeevent can still invoke this direct listener and schedule a new timeout. This is reachable when a change subscriber disposes the storage synchronously because the emitter continues its current listener snapshot; guardarmWatchdogso cleanup cannot be undone.
const armWatchdog = () => {
clearTimeout(watchdog);
watchdog = setTimeout(() => {
pull.cancel();
finish();
}, watchdogMs);
};
packages/modules/state/src/storage/PouchDbSyncStorage.ts:271
- The split pull path likewise never merges
syncOptions.pull, so valid pull-only filters, query parameters, and timeouts that worked withdb.sync()are ignored as soon as scheduled mode is selected. Build effective pull options from the top-level and nested pull settings, and use the same effective timeout for the watchdog below.
{
...this.#syncOptions,
live: false,
retry: false,
// Guarantees 'complete'/'error' fires even against a backend that never answers a
// one-shot request - otherwise a single hung poll would wedge #pullInFlight forever,
// silently turning every later timer/focus trigger into a no-op skip.
timeout: this.#syncOptions.timeout ?? 30000,
},
packages/modules/state/src/events/index.ts:55
- The published sync-event catalog in
packages/modules/state/docs/events.md:69-87still lists only four event kinds and saysStateSyncEvent.ismatches four events. AddingPollhere makes that consumer-facing reference incorrect; addonStateSync.polland its payload to the table and update the count.
Poll: StateSyncPollEvent,
packages/modules/state/src/storage/PouchDbSyncStorage.ts:219
SyncOptionssupports per-directionpushoverrides, but passing the whole object toreplicate.to()does not mergesyncOptions.pushasdb.sync()does. In scheduled mode, valid push-only filters, query parameters, and retry settings are therefore silently ignored; merge the nested push options before forcing the live invariant.
This issue also appears in the following locations of the same file:
- line 263
- line 300
- line 353
{
...this.#syncOptions,
live: true,
retry: this.#syncOptions.retry ?? true,
},
- Files reviewed: 13/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
- watchdog progress test: the captured 'change' handlers include observePouchDbReplicate's
own onChange, which reads change.docs directly - calling handlers with no argument crashed
it. Pass a { docs: [] } payload, matching the pattern used elsewhere in this file.
- public sync() test: depended on a real, live, continuous db.sync() connection outliving
the test via real timers, which could hang the local db's destroy() in afterEach. Mock
push, pull, and sync so the test is fully deterministic under fake timers.
Disposing while iterating `#teardown` directly could skip an entry when a teardown callback itself deregistered a sibling entry (e.g. `_pullOnce`'s `finish()`), since splicing the array mid-iteration shifts later entries into the index the iterator already passed. Snapshot-and-clear via `splice(0)` before iterating avoids the skip and makes double-dispose a no-op.
…tchdog rearm - Add #effectiveReplicateOptions(direction) to merge syncOptions.push/ syncOptions.pull onto the shared options, the same way db.sync() applies them internally. _startLivePush/_pullOnce previously replaced db.sync() with separate replicate.to/replicate.from calls but never applied these per-direction overrides, silently dropping a caller's push- or pull-only filters, query params, or timeout. The pull watchdog now also derives its deadline from this same effective timeout. - Guard armWatchdog with the existing 'settled' flag: a 'change' event already dispatching when finish() runs elsewhere in the same tick could otherwise re-arm a new timeout right after cleanup cleared it, leaking an orphaned timer past the pull's own completion.
Regression test for the teardown-skip bug fixed in PouchDbStorage.ts: disposes storage while a hung pull's teardown is still registered, then asserts the scheduled pull interval was actually stopped (no further replicate.from calls), not just the in-flight pull's own cancel.
The sync-events table and 'four sync events' count text predated StateSyncPollEvent being added to the StateSyncEvent union - add it as a fifth row plus a usage example, matching the existing events' flattened property-access style (event.trigger, event.skipped).
The dispose-loop snapshot fix now reliably runs every teardown instead of sometimes skipping one - which surfaced that this test's mocked db.sync() result was missing removeListener, throwing when the sync subscription's teardown actually ran on dispose.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Why is this change needed?
PouchDbSyncStoragealways used a single continuous bidirectionaldb.sync()connection. At production user counts this means every idle client keeps a live_changeslongpoll open for pull, a direction that's rarely needed in real time.What is the current behavior?
Sync always opens one continuous
db.sync()connection per client for both push and pull, for the lifetime of the storage instance.What is the new behavior?
A new
pulloption onPouchDbSyncStoragecontrols how the pull direction is scheduled, independent of push:mode: 'live'(default, unchanged) - continuous bidirectionaldb.sync(), exactly as before.mode: 'interval'- push stays live viadb.replicate.to; pull runs as one-shotdb.replicate.fromcalls on a timer (intervalMs, default 60s) and on tab focus (unlessrefreshOnFocus: false).mode: 'visible-interval'- same as'interval', but skips the timer tick entirely while the tab is hidden (Page Visibility API), since a backgrounded tab has no user waiting on fresh data.createDefaultStorage()now usespull: { mode: 'visible-interval', refreshOnFocus: true }, and is also exported from@equinor/fusion-framework-module-state/default-storageso callers can reuse the framework's default remote-resolution behavior (service discovery, auth, per-user CouchDB proxy) with their ownpulloverrides, e.g. a shorter interval for previewing.The
app-react-statecookbook now uses this to preview with a 10s interval instead of production's 60s default, and itsSyncStatusIndicatorrecognizes the newonStateSync.pollevent.What is the intended behavior or invariant?
#pullInFlight); a trigger that arrives while one is running is skipped, not queued or overlapped.#pullInFlight, even if PouchDB never fires'complete'/'error'and never settles its thenable interface - a watchdogsetTimeout(syncOptions.timeout ?? 30000+ 5s) force-cancels the replication and releases the guard as a last resort.'complete','error', the thenable, and the watchdog all race to call the samefinish(), guarded to run exactly once.pull.mode, so local writes are never delayed by pull scheduling.Does this PR introduce a breaking change?
No.
pullis optional and defaults tomode: 'live', which preserves the exact prior behavior for any caller not opting in.Impact assessment:
@equinor/fusion-framework-module-state), Patch (cookbook)setStoragewith an explicitPouchDbSyncStorageand nopulloption are unaffected.@equinor/fusion-framework-module-stateand theapp-react-statecookbook.Review guidance:
_pullOnce()(PouchDbSyncStorage.ts) is the main thing worth scrutinizing - specifically thatfinish()can only run once and always clears the timeout.describe('pull watchdog', ...)) exercises the watchdog path via fake timers and a mocked hungreplicate.fromcall. It compiles cleanly (tsc -b --force) but could not be executed in this sandbox -leveldown's native binding has no prebuilt binary for this environment's Node/arch combination, which blocks the entire package's existing Vitest suite too (not something introduced by this PR). CI should run it.Additional context
See
.changeset/module-state_interval-pull.mdand.changeset/cookbook-app-react-state_poll-preview.mdfor the full consumer-facing changelog text.Related issues
None.
Checklist